perf: speed up path conflict detection#29
Conversation
The traversal queues regrew from nil on every path insertion, which accounted for ~90% of allocated bytes. BenchmarkConflictTree/pkgs=1000: -91% B/op, -37% allocs/op, -17% sec/op.
A literal segment scanned every sibling node linearly even though Children is keyed by segment text. Look up the exact child directly and scan only wildcard children, tracked in a new GlobChildren list. BenchmarkConflictTree/pkgs=1000: -70% sec/op; DeepPaths: -85% sec/op.
letFunny
left a comment
There was a problem hiding this comment.
Thank you looking into this Marcin, I added comment about each improvement but basically IMO one of them should certainly go in, and for the other one we can discuss it.
With that said, I think it is better if we wait for the original PR to be merged. I spoke with Gustavo and he was very clear in that he didn't want to pile up more improvements; he wanted to have a clear algorithm first then improve it later. Let's do the following, once the original PR is merged you can create a new PR with these improvements, I wouldn't want to claim authorship :)
| type pathConflictTree struct { | ||
| Root *node | ||
| PathToSlices map[string][]*Slice | ||
| // currentQueue and nextQueue are kept across pathHasConflict calls so |
There was a problem hiding this comment.
When writing the original PR I cached the queues, then I removed it, then added it back, etc. and in the end I don't have a strong opinion on whether to do it or not. It obviously increases performance (around 20% on my machine, so 20ms out of 100ms). The only reason I didn't add it from the get go was a similar case in the past where there was an optimization by caching the results and Gustavo argued against it as it increased the complexity.
After seeing you came up with the same idea I think we should start the debate again.
| Segment segment | ||
| SegmentSlices []*segmentSlice | ||
| Children map[string]*node | ||
| // GlobChildren lists the children whose segment contains a wildcard, so |
There was a problem hiding this comment.
This is very interesting. I didn't expect such a simple change to have 25% improvement (my machine). I had done another more complex optimization for looking for candidates that had the same % improvement but the implementation was far worse.
I think we should go for this one.
|
yep. no problem :)) happy to submit it afterwards and yeah, i agree that it's better to split them. i wanted to look into this now in case 1) there would be a l-line improvement or 2) there would be a bigger perf gain but behind some slight reengineer which might as well land at this stage. none of those are the case and yeah, this PR can easily go in afterwards. re the improvements here; they are somewhat coupled, bit throughout but through effect. i did add this line from my initial message, but it's just one and maybe i should have been clearer about it:
fix 2 by itself works ok, but does induce much more GC pressure w/out fix 1. tbd after your bade PR is merged and we send this one in, i think 👍 |
two optimisations for
pathConflictTree, on top of the trie from this branch. no behaviour change intended: same conflicts detected, same error type. tests pass unchanged. checked with my fuzz tests too.1. reuse the traversal queues (
f449d7a)pathHasConflictused to growcurrentQueueandnextQueuefrom nil on every path insertion, and seeded the first level withslices.Collect(maps.Values(...)). with a few thousand paths this dominated allocation: the queues were the single largest source of gc work in the whole conflict check.i've changed
currentQueueandnextQueueto fields onpathConflictTree, reset with[:0]per call and restored through adefer, so the backing arrays are grown once and reused.proof of gc work (drop somewhere in
internal/setup):after:
2. exact-match child lookup (
3c097a7)when a segment matched, the old traversal enqueued every child of the matched node and compared each one against the next segment on the following round. but
Childrenis already keyed by segment text, so for a literal next-segment the only children that can possibly match are the one with the identical text andany child whose segment holds a wildcard.
now
appendCandidatesdoes and exact map lookup, plus a scan of the newnode.GlobChildrenlist. a wildcard next-segment still considers every child, since it can match anything. glob-vs-glob comparisons are unchanged.this turns the linear sibling scan in wide directories into a map lookup. real chisel-releases paths are at present aboout ~60% literal directories and this speeds this up, especially because the wide nodes tend to also be mostly literal so the quadratic scan hurts a lot there).
proof, with fix 1 above applied, otherwise GC noise swamps the signal:
after: